fix(pve): serve PVE requests under /api/pve and drop per-gateway rewrites#6130
fix(pve): serve PVE requests under /api/pve and drop per-gateway rewrites#6130xuang7 wants to merge 6 commits into
Conversation
Automated Reviewer SuggestionsBased on the
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…xuang7/texera into fix/single-node-nginx-pve-route
| proxy_set_header Host $host; | ||
| } | ||
|
|
||
| location /pve/ { |
There was a problem hiding this comment.
we should change this to /api/pve/ in the src code instead of doing a mapping here.
There was a problem hiding this comment.
Agreed. No other comments for me.
Per review: the frontend now calls /api/pve directly via AppSettings.getApiEndpoint(), so the /pve-to-/api/pve rewrite rules in single-node nginx, the dev proxy, and the k8s gateway become plain routing entries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6130 +/- ##
============================================
+ Coverage 60.26% 60.64% +0.38%
Complexity 3339 3339
============================================
Files 1133 1139 +6
Lines 43979 44874 +895
Branches 4781 4930 +149
============================================
+ Hits 26503 27214 +711
- Misses 16021 16173 +152
- Partials 1455 1487 +32
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
v1.2 adaptation of b864745 on main (PR apache#6130): the frontend builds PVE URLs with AppSettings.getApiEndpoint(), so single-node nginx, the dev proxy, and the k8s gateway all become plain routes with no rewriting. Fewer files than on main because this branch predates the StoredPve db endpoints and the gateway chart reorganization. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes single-node and gateway routing for Python Virtual Environment (PVE) REST calls by aligning the frontend to call PVE endpoints under the backend’s /api mount (i.e., /api/pve/...) and removing per-gateway /pve -> /api/pve rewrite special-casing across deployment configs.
Changes:
- Update the frontend PVE service to build REST URLs using
AppSettings.getApiEndpoint()(so requests go to/api/pve/...). - Update local dev proxying and single-node NGINX to route
/api/pvedirectly to the coordinator service. - Update the Kubernetes gateway HTTPRoute to match
/api/pveand drop the rewrite filter.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/workspace/service/virtual-environment/virtual-environment.service.ts | Switch PVE REST calls to use AppSettings.getApiEndpoint() so they resolve under /api/pve/*. |
| frontend/src/app/workspace/service/virtual-environment/virtual-environment.service.spec.ts | Update unit tests to match the new base URL construction. |
| frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts | Update comment to reflect /api/pve/system endpoint path. |
| frontend/proxy.config.json | Add /api/pve proxy target to the coordinator and remove the old /pve rewrite entry. |
| bin/single-node/nginx.conf | Add explicit location /api/pve/ proxying to the workflow-runtime coordinator. |
| bin/k8s/templates/base/gateway/gateway-routes.yaml | Add /api/pve to dynamic backend matches and remove the /pve rewrite rule. |
Comments suppressed due to low confidence (1)
frontend/src/app/workspace/service/virtual-environment/virtual-environment.service.spec.ts:85
- This spec only covers the CRUD
/pve/dbmethods, but this PR also changed the request URLs forgetSystemPackages()andfetchPVEs()(and related helpers). Adding unit tests for those methods would prevent regressions in the new/api/pve/*routing (URL + requiredcuidquery param).
it("deleteUserPve() DELETEs /pve/db/{veid}", () => {
service.deleteUserPve(9).subscribe();
const req = httpTestingController.expectOne(`${AppSettings.getApiEndpoint()}/pve/db/9`);
expect(req.request.method).toBe("DELETE");
req.flush(null);
});
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tching Mirrors the same change on the main PR (apache#6130) to keep the two branches identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tching Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
95e2bda to
e7ee16c
Compare
…tching Mirrors the same change on the main PR (apache#6130) to keep the two branches identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What changes were proposed in this PR?
The single-node NGINX gateway (
bin/single-node/nginx.conf) had no route for the Python Virtual Environment (PVE) REST calls, so they fell through to the catch-alllocation /and returned 404 fromdashboard-service. In the UI this surfaces as a "Could not load Python virtual environments" popup on every Python UDF operator selection.The root cause is that the frontend requested
/pve/...while the backend (PveResource,@Path("/pve"), Jersey mounted under/api/*on the coordinator) serves/api/pve/..., so every gateway had to carry its own/pveto/api/pverewrite rule, and the single-node config was missing one.Per review feedback, instead of adding the missing rewrite, this PR removes the mismatch at the source: the frontend now builds PVE URLs with
AppSettings.getApiEndpoint()and requests/api/pve/...directly. Every routing layer then becomes a plain routing entry with no path rewriting:bin/single-node/nginx.conf:location /api/pve/passes through to the coordinatorfrontend/proxy.config.json:/api/pveentry (ordered before/api),pathRewriteremovedbin/k8s/templates/base/gateway/gateway-routes.yaml:/api/pvejoins the existing dynamic-backend match list, theURLRewritefilter is removedThe backend is unchanged. Net effect: the 404 is fixed, and the per-gateway rewrite special-casing that allowed the three configs to drift apart is gone.
Any related issues, documentation, discussions?
Related to #5963 (PVE single-node configuration).
How was this PR tested?
virtual-environment.service.spec.tspasses with the new URLs (5 specs).GET /api/pve/pves?cuid=1returns 200 JSON from the coordinator, and the old/pvepath falls through to the dashboard as expected.helm templaterenders the updated k8s route with theURLRewritefilter removed.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8, Claude Fable 5)